-
Notifications
You must be signed in to change notification settings - Fork 62
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
add support for app role assignments for users, groups and sp #39
add support for app role assignments for users, groups and sp #39
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for looking at this @michaljirman.
I was thinking about app role assignments recently and wondered if it would make more sense to implement all of the functionality as part of the GroupsClient, ServicePrincipalsClient and UsersClient structs rather than having separate clients. WDYT?
@manicminer implementing method 2 within the GroupsClient, ServicePrincipalsClient and UsersClient will result in 3 near-identical methods in each of them and a lot of code duplication (appRoleAssignments relationship). We could still consider this refactor but it would be good to keep abstraction of these methods somewhere else and just call it from all of these clients. If we want to also keep method 1 we will additionally have extra 3 methods in the ServicePrincipalsClient (appRoleAssignedTo relationship) |
@manicminer we should probably get this reviewed/updated before I forget what it does :-) |
@michaljirman Sorry for the delay in reviewing this, I've been focused on fixes and updates for the AzureAD Terraform provider the last 2 weeks. I'm planning to get this in along with #42 in the next few days. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks again @michaljirman, this LGTM 🚀
Supports app role assignments using the two available methods:
appRoleAssignedTo relationship
client example
appRoleAssignments relationship
client examples
Related to this issue